Skip to content

feat: refactor partitioners - #3847

Closed
bd713 wants to merge 42 commits into
developfrom
feature/bd713/enhancePartitioner
Closed

feat: refactor partitioners#3847
bd713 wants to merge 42 commits into
developfrom
feature/bd713/enhancePartitioner

Conversation

@bd713

@bd713 bd713 commented Oct 5, 2025

Copy link
Copy Markdown
Contributor

Description

This PR introduces a complete refactoring of GEOS's mesh partitioning system with a new architecture that separates partitioning algorithms (engines) from domain decomposition strategies (partitioners).

In practice, a new DomainPartitioner abstract base class is introduced with two main specialized branches:

DomainPartitioner (Abstract Base)
├── GeometricPartitioner (Abstract)
│   ├── CartesianPartitioner (Concrete)
│   └── ParticleCartesianPartitioner (Concrete)
└── MeshPartitioner (Abstract)
    ├── CellGraphPartitioner (Concrete)
    └── LayeredMeshPartitioner (Concrete)

The mesh partitioners leverage low-level graph partitioner engines that are independent from MPI communication:

GraphPartitionEngine (Abstract Engine Interface)
├── ParMetisEngine (Concrete)
├── PTScotchEngine (Concrete)
└── NoOpEngine (Concrete)

A PartitionerManager is introduced for creating and managing partitioner instances.

XML Changes

Partitioning configuration moved from mesh-embedded to dedicated <Partitioner> section.

Before:

<Mesh>
   <VTKMesh
       file="mesh.vtu"
       partitionMethod="parmetis"
       partitionRefinement="2"  />
</Mesh>       

After:

<Partitioner>
  <CellGraphPartitioner engine="parmetis" numRefinements="1"/>
</Partitioner>
<Mesh>
  <VTKMesh file="mesh.vtu"/>
</Mesh>

Default behavior
If no partitioner is prescribed, an appropriate default is selected based on mesh type:

  • Internal meshes: CartesianPartitioner
  • External meshes: CellGraphPartitioner with ParMetis

To skip graph partition optimization, explicitly use engine="noop".

Testing & Rebaseline

Due to XML schema changes a rebaseline is required.
However no numerical impact is expected.

INFO: Total number of log files processed: 1047

INFO: No unfiltered differences were found.

All currently failing integrated tests are due to structural changes such as:

********************************************************************************
Error: /Problem
	Group has a child 'Partitioner' in the file to compare but not the baseline file.
********************************************************************************
********************************************************************************
Error: /Problem/domain
	Group has a child 'partitionManager' in the baseline file but not the file to compare.
********************************************************************************

@bd713 bd713 removed the flag: requires rebaseline Requires rebaseline branch in integratedTests label Oct 12, 2025
DENEL Bertrand added 2 commits October 13, 2025 23:45
@bd713
bd713 force-pushed the feature/bd713/enhancePartitioner branch from 45ad26f to 5ce56de Compare October 22, 2025 17:54
@bd713 bd713 added flag: ready for review flag: requires rebaseline Requires rebaseline branch in integratedTests and removed DO NOT MERGE ! labels Oct 25, 2025
@paveltomin

Copy link
Copy Markdown
Collaborator

@bd713 why did you change some tests inputs? defaults should be fine, no?

@bd713

bd713 commented Nov 4, 2025

Copy link
Copy Markdown
Contributor Author

@bd713 why did you change some tests inputs? defaults should be fine, no?

For external meshes, the default remains unchanged: ParMetis is run once with no additional refinement iteration.
Hence, only tests that relied on a non-default partitionRefinement must be updated.

Matrix conversion:
partitionRefinement=0 => Explicitly use NoOp engine
partitionRefinement=1 (default) => No changes needed, the new default numRefinements=0 will be used, following Metis convention
partitionRefinement=3 (i.e. run ParMetis + 2 extra refinements) => Explicitly set numRefinements=2

We also have 1 test using ptscotch that needs to be edited as well.

@MelReyCG MelReyCG left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good to me, but I do not have knowledge in partitioners, I left some remarks:

  • Should the package name be something like partitioner rather than mpiCommunications (or could it be splitted?)

using namespace dataRepository;

class SpatialPartition;
//class SpatialPartition;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove?

* @brief Get partition counts in all directions
* @return Array of partition counts {nx, ny, nz}
*/
array1d< int > const & getPartitionCounts() const { return m_partitionCounts; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this array size is not fixed?

Suggested change
array1d< int > const & getPartitionCounts() const { return m_partitionCounts; }
R1Tensor const & getPartitionCounts() const { return m_partitionCounts; }

or

Suggested change
array1d< int > const & getPartitionCounts() const { return m_partitionCounts; }
stackArray1D< int, 3 > const & getPartitionCounts() const { return m_partitionCounts; }

Comment on lines +30 to +32
// Modulo
// returns a positive value regardless of the sign of numerator
real64 Mod( real64 const num, real64 const denom )

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doxygen format?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(here and below)

*/
unsigned int getZPartitions() const { return m_partitionCounts[2]; }

real64 const * getLocalMin() const { return m_localMin;}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels unsafe to return an unsized array through a pointer.

array1d< int > m_partitionCounts;

/// Local subdomain minimum (x, y, z)
real64 m_localMin[3];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stackArray1D< int, 3 > ?

Comment on lines +38 to +45
/**
* @class GraphPartitionEngine
* @brief Abstract interface for low-level graph partitioning algorithms
*
* This is a LOW-LEVEL engine for pure algorithms: graph -> partition IDs
*/
class GraphPartitionEngine : public dataRepository::Group
{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really have to be a Group, or should it be an interface (abstract pure class / templated variant)?

{

class SpatialPartition;
//class SpatialPartition;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove?

Comment on lines +78 to +79
ArrayOfArraysView< pmet_idx_t const, pmet_idx_t > const & graph,
arrayView1d< pmet_idx_t const > const & vertDist,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of const-cast. Should we remove those const qualifier?

@bd713 bd713 closed this May 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes XML input ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: ready for review flag: requires rebaseline Requires rebaseline branch in integratedTests type: cleanup / refactor Non-functional change (NFC)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants